You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Fixture creation (clone + install NC server, dumps, app install) takes ~30–55 s per job. The fixture is now archived with sudo tar (keeps ownership of root-owned DB files) and cached with actions/cache, keyed on DB (incl. image and env files), PHP, NC branch commit, app migrations, appinfo/info.xml and the test runner. On a hit, only the fixture is restored. The cache is saved right after fixture creation, even if tests fail.
Job
Before
After (cache hit, + ~7 s restore)
mysql
32 s
9 s
sqlite
27 s
4 s
pgsql
34 s
10 s
The MySQL healthcheck now pings over TCP: the socket ping reported healthy on the temporary init server, which a fast restore exposed.
Concerns/issues
NC master job not cached: master moves too often for cache hits.
Each entry is ~430 MB (10 GB repo limit) and changes whenever the NC branch moves.
Formal requirements
I did check that the app can still be opened and does not throw any browser logs
I created tests for newly added PHP code (check this if no PHP changes were made)
I updated the OpenAPI specs and added an entry to the API changelog (check if API was not modified)
I notified the matrix channel if I introduced an API change
Run unit, integration and migration tests as separate steps to expose per-suite timings.
Collect coverage XML from all runs since each call creates its own run folder.
Assisted-by: Claude Code:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Léo Jan <leo.jan@doctolib.com>
Restore the fixture instead of reinstalling the Nextcloud server when the server commit, app migrations and test runner are unchanged. Skipped for pgsql, whose fixture holds root-owned database files.
Assisted-by: Claude Code:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Léo Jan <leo.jan@doctolib.com>
The cache post step re-evaluated hashFiles and failed on root-owned files created in the test volumes.
Assisted-by: Claude Code:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Léo Jan <leo.jan@doctolib.com>
Otherwise docker creates them as root when starting the helpers, and the restore cannot change their ownership.
Assisted-by: Claude Code:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Léo Jan <leo.jan@doctolib.com>
The socket ping reported healthy on the temporary init server, so a quick restore of a cached fixture hit the restart window.
Assisted-by: Claude Code:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Léo Jan <leo.jan@doctolib.com>
It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.
No actionable comments were generated in the recent review.
Recent review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 63ab8f67-dff4-4c91-8176-0c8fa2bf17e2
Commits
Reviewing files that changed from the base of the PR and between 5882ba6 and 6d53d36.
Files selected for processing: 1
.github/workflows/tests.yml
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review.
Walkthrough
The test workflow now caches fixture archives. The cache key uses database, core, and PHP versions, selected file hashes, the current UTC week, and the stable server commit. When the cache is usable, the workflow extracts fixtures and starts helpers. Otherwise, it creates, archives, and saves fixtures. The MySQL health check now runs mysqladmin ping against 127.0.0.1 instead of using the Unix socket.
Priority: Low
Merge Risk: Low · up to 6d53d
The workflow remains functional, but fixture caches cannot be reused across pull requests without a seeded base-branch cache, limiting the intended setup-time savings.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title clearly identifies the main change: caching test fixtures between CI runs.
Description check
The description is complete and relevant. It explains the scope, cache behavior, performance impact, MySQL health-check change, known concerns, linked issue, and formal requirements.
Linked Issues check
Issue #2722 ("Optmize Fixtures") asks that fixtures be cached to speed up recreating them. It sets no further acceptance criteria. The whole-PR diff (merge base 665c61e to head 6d53d36) of .github/wor…
Out of Scope Changes check
The whole-PR diff contains two changed files. The first is .github/workflows/tests.yml, which contains only the fixture cache steps described under #2722. The second is .github/actions/run-tests/mysql…
Docstring Coverage
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
Full details: Linked Issues check
Explanation
Issue #2722 ("Optmize Fixtures") asks that fixtures be cached to speed up recreating them. It sets no further acceptance criteria. The whole-PR diff (merge base 665c61e to head 6d53d36) of .github/workflows/tests.yml implements this request. It adds a cache key step (DB, core version, PHP version, hashed fixture-relevant files, UTC week, and the stable-branch commit from git ls-remote). It adds an actions/cache/restore step. On a usable hit, it extracts fixture.tar and starts helpers with --start-helpers instead of creating the fixture. On a miss, it archives version.json and fixtures/default with sudo tar, and saves the cache with actions/cache/save before the unit tests run. The save step runs even when the unit tests fail. The PR description lists the key inputs but omits the UTC week and the restore-key fallback. The workflow implements both. This difference does not change the verdict. The issue has no automated test requirement.
Full details: Out of Scope Changes check
Explanation
The whole-PR diff contains two changed files. The first is .github/workflows/tests.yml, which contains only the fixture cache steps described under #2722. The second is .github/actions/run-tests/mysql/Dockerfile. Its HEALTHCHECK changes from mysqladmin ping --socket /run/mysqld/mysqld.sock to mysqladmin ping -h 127.0.0.1. The PR description states that a socket ping reported healthy on the temporary initialization server, and that a fast restore exposed this. The TCP check therefore supports the caching change. No unrelated change appears in the reviewed diff. The restore-key fallback for pull requests can reuse a fixture built from an earlier stable-branch commit. The PR description states this design intentionally, so it is not an out-of-scope change.
Full details: Docstring Coverage
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.)
Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Would it make sense to tar the cache content? This should allow us to preserve the permissions. Also, it is faster to create and download artifacts with a low number of entries (heuristic finding>
ah yeah tar is a good idea, for permissions, and also we can now include pgsql!
actions/cache packs all the cached paths into one zstd tarball before uploading, so the cache is a single entry no matter how many files the fixture has. The per-file overhead you have in mind applies to upload-artifact, not to actions/cache, so tarring first wouldn't change the speed.
Tarring with sudo keeps ownership of the root-owned pgsql DB files, which actions/cache could not read.
Assisted-by: Claude Code:claude-opus-5-5
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Léo Jan <leo.jan@doctolib.com>
Aah, right, it was about artifacts. I built an image on docker hub from this branch and I'm fact it seems to speed things up. See the MariaDB run.
I will remove the permission rule again to not forget it.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: dc0ce68b-31e8-450b-ad58-8440ef6d2b63
Commits
Reviewing files that changed from the base of the PR and between 665c61e and 97bf3fe.
Files selected for processing: 2
.github/actions/run-tests/mysql/Dockerfile
.github/workflows/tests.yml
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
Follow-up Comments
Synchronize the cookbook app on cache hits.
239-239: Major · Functional Correctness · Quick win
Synchronize the cookbook app on cache hits.
The cache-hit branch only creates directories and starts helpers. It does not run the app synchronization performed during fixture creation. The archive contains only version.json and fixtures/default, while Docker Compose mounts volumes/cookbook over /var/www/html/custom_apps/cookbook. A fresh cache-hit runner therefore mounts an empty cookbook app, so the tests cannot load the current app code. Synchronize the checkout into volumes/cookbook before starting the helpers, or include that directory in a cache design keyed by the app revision. (raw.githubusercontent.com)
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 3adc56c6-c08b-4af7-9768-b896b06dfee1
Commits
Reviewing files that changed from the base of the PR and between 97bf3fe and 2f4d3e2.
Files selected for processing: 1
.github/workflows/tests.yml
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.
there is a catch: the pr would always have a cache miss on the initial ci runsince the cache is only created from the pr (this doesnt run on master)
So we see the benefit only on a second run from a PR, wich is not ideal I guess
i don't have an ideal solution to fix that actually (i don't want to make the whole CI run on master, it would be costly
But yeah we could either run the ci on master, or a daily cron job , or use a daily key for the cache : A PR's first run each day misses, then every later push or rerun that day hits (that is the cheapest option with higher gain, for a daily approach)
The point with master is #3074. So, there might already be a benefit on this (even if it is just about letting the corresponding master run be faster). Also, typically there more than one commit on a branch (at least if there are actual test changes involved). So, I think we can live with that for the meantime. We just have to think about a (good) key naming scheme such that we can reach it "anywhere".
Can we eventually use a hash to check the fixture's validity somehow? Just thinking loudly...
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 054f29e3-67d7-487d-9951-7c1175c5a8aa
Commits
Reviewing files that changed from the base of the PR and between 2f4d3e2 and 1d1081b.
Files selected for processing: 1
.github/workflows/tests.yml
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
what about the following behavior (that is in this PR already):
we have a cache name, hashing the important stuff for tests (DB, NC branch, PHP, file hashes, current week), and server commit
restore-keys: if there is no exact match (server commit changed), we can reuse a cached fixture that is good enough (different server commit but the rest is same: db, branch, php and so on). The week in the name means a reused fixture is never older than the current week.
On master (once #3074 runs CI there), the cache is rebuilt instead of reused, to have an exact match. So PRs can reuse a cache a few hours old
Also, CI was red because some jobs ran on the garm runners: phpunit there exits immediately without running any test (only the coverage copy step catches it). The jobs on GitHub-hosted runners pass.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review info
Run configuration
Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 8c41c100-032f-4be3-9728-14a0ff348e6b
Commits
Reviewing files that changed from the base of the PR and between 1d1081b and 5882ba6.
Files selected for processing: 1
.github/workflows/tests.yml
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Closes #2722
Topic and Scope
Fixture creation (clone + install NC server, dumps, app install) takes ~30–55 s per job. The fixture is now archived with
sudo tar(keeps ownership of root-owned DB files) and cached withactions/cache, keyed on DB (incl. image and env files), PHP, NC branch commit, app migrations,appinfo/info.xmland the test runner. On a hit, only the fixture is restored. The cache is saved right after fixture creation, even if tests fail.The MySQL healthcheck now pings over TCP: the socket ping reported healthy on the temporary init server, which a fast restore exposed.
Concerns/issues
Formal requirements
AI disclosure
I used AI for code gen.